fix(auth): guard OAuth :strategy route — 404 unknown/disabled + stateless authenticate#3947
Conversation
…less authenticate
oauthCall and oauthCallback passed req.params.strategy straight into
passport.authenticate() with no validation: an unknown strategy made
passport throw synchronously ("Unknown authentication strategy") -> 500,
and a registered-but-unguarded provider defaulted to session:true, which
fails on this stateless JWT stack (no express-session mounted).
Add isEnabledOAuthProvider(), shared by both routes: allowlisted (in
ALLOWED_PROVIDERS) AND actually registered with passport
(passport._strategy(strategy)) -> otherwise reject with a clean 404
(OAUTH_PROVIDER_NOT_FOUND) before ever reaching passport.authenticate().
oauthCall now also passes { session: false } explicitly.
Update the invitations alias-shadow test to assert on the new deliberate
404 (code) instead of "not 404", and add strategy-registration stubs to
the existing oauthCallback integration tests so they keep exercising the
post-authenticate path. New unit tests cover unknown/disabled/enabled
strategies for oauthCall.
…ession test Adversarial review of the #3900 guard diff found the oauthCallback reject branch had zero coverage (all 6 existing tests stub _strategy truthy) and that the pre-existing "should NOT mint a session" security regression test was silently defeated: since google is unregistered in test config, the new guard now 404s before the request ever reaches the post-authenticate identity-trust logic the test protects. - Add oauthCallback guard-reject unit tests (unknown + allowlisted-but- unregistered strategy), mirroring the existing oauthCall coverage. - Stub passport._strategy + passport.authenticate in the session-mint regression test so it again reaches the post-auth "no user" branch instead of short-circuiting at the guard; assert the redirect carries the no-user error, never OAUTH_PROVIDER_NOT_FOUND. - Reuse the existing 'oAuth, unsupported provider' AppError message for both new guards instead of a fresh string, for consistency. - Add a describe-scoped afterEach(jest.restoreAllMocks()) safety net so a failing assertion mid-test can't leak a stubbed spy into later tests (clearMocks alone doesn't restore spy implementations).
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR adds an allowlist/registration guard for OAuth providers in the auth controller, rejecting unknown or unregistered strategies with a 404 before invoking Passport, switches ChangesOAuth Provider Guard
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant Passport
Client->>AuthController: GET /api/auth/:strategy
AuthController->>AuthController: isEnabledOAuthProvider(strategy)
alt provider unknown or not registered
AuthController-->>Client: 404 OAUTH_PROVIDER_NOT_FOUND
else provider allowed
AuthController->>Passport: authenticate(strategy, {session:false})
Passport-->>Client: redirect to provider / callback
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3947 +/- ##
=======================================
Coverage 92.69% 92.70%
=======================================
Files 169 169
Lines 5557 5562 +5
Branches 1787 1789 +2
=======================================
+ Hits 5151 5156 +5
Misses 326 326
Partials 80 80
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/auth/controllers/auth.controller.js`:
- Around line 442-466: The isEnabledOAuthProvider guard currently assumes
passport._strategy exists and is callable, so a Passport upgrade or mock can
make it throw instead of failing closed. Update the auth.controller.js check
around isEnabledOAuthProvider to verify typeof passport._strategy === 'function'
before calling it, or switch to a locally tracked registry of enabled
strategies, so the /api/auth/:strategy routes still return false for
unknown/disabled providers without risking an exception.
In `@modules/auth/tests/auth.oauthCall.controller.unit.tests.js`:
- Around line 24-110: The `beforeEach` mock setup in the `oauthCall` and
`oauthCallback` test blocks is duplicated, so extract the shared
`jest.unstable_mockModule` initialization into a reusable helper such as
`setupAuthControllerMocks()` in a test-utils module. Keep the existing mock
behavior for `mockPassport`, logger, config, users, organizations, helpers, and
analytics, and have both describe blocks call the helper instead of repeating
the same setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f44fc14a-13f7-46b3-893b-256bcd382918
📒 Files selected for processing (4)
modules/auth/controllers/auth.controller.jsmodules/auth/tests/auth.integration.tests.jsmodules/auth/tests/auth.oauthCall.controller.unit.tests.jsmodules/invitations/tests/invitations.integration.tests.js
… (CodeRabbit #3947) Deduplicates ~85 lines of identical jest.unstable_mockModule setup that was copy-pasted across the oauthCall and oauthCallback describe blocks' beforeEach hooks into a single setupAuthControllerMocks() helper, called from both. Placed under tests/fixtures/ (matching lib/middlewares/tests/fixtures/) so Jest's testPathIgnorePatterns keeps it out of testMatch — a tests/helpers/ name would have made Jest try to run it as its own (empty) test suite.
Summary
GET /api/auth/:strategyand its/callbackroute no longer passreq.params.strategystraight intopassport.authenticate(). A newisEnabledOAuthProvider()guard rejects any strategy name that is not both allowlisted (ALLOWED_PROVIDERS) and actually registered with passport, returning a clean404(OAUTH_PROVIDER_NOT_FOUND) instead of a synchronous throw.oauthCallnow also passes{ session: false }topassport.authenticate()(stateless JWT stack, noexpress-sessionmiddleware mounted)./api/auth/*was reachingpassport.authenticate(strategy)unguarded. An unknown name (e.g. probe traffic hitting/api/auth/me) made passport throw synchronously ("Unknown authentication strategy") → 500. A provider that's allowlisted but not registered (missing credentials) hit the same throw. An actually-registered provider defaulted tosession: trueand would fail immediately with "Login sessions require session support" on a stack with no session middleware. This closes all three off as clean, testable 404s / correct stateless auth.Scope
modules/auth(controller only — no route/schema changes)none— the one line touched outsidemodules/authis a pre-existingmodules/invitationsintegration test assertion that exercises the same/api/auth/googleroute; it now asserts the new deliberate 404 contract instead of "any non-404", noinvitationsmodule logic changed.lowValidation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Reviewer notes
passport._strategy(strategy)— this is@api privatein passport, but it is the only source-of-truth for "is this strategy registered" in passport@0.7.0 (no public accessor exists; onlyuse()/unuse()). A config-based check would duplicate each provider's own credential-presence gating (strategies/local/{google,apple}.js) and risk drift. Deliberate trade-off, documented inline onisEnabledOAuthProvider().{ session: false }is passed topassport.authenticateinoauthCall(the defaultsession: truewas the root cause of the "Login sessions require session support" error on this stateless-JWT stack). It is intentionally not added tooauthCallback— passport's callback form (passport.authenticate(strategy, callback)) never callsreq.logIn()/ readsoptions.session, so it would be a dead option there. Both are commented inline for the next reader.oauthCallback's 404-guard branch is now covered by the new unit test file, and a pre-existing session-mint regression test (identity-trust logic) had its coverage restored past the new guard so it still reaches the code it was written to protect.Notes for reviewers
modules/auth, no sharedlib//config/changes.Summary by CodeRabbit
Bug Fixes
Tests